-
Notifications
You must be signed in to change notification settings - Fork 898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add toJSON() support for Firestore classes #4298
Conversation
🦋 Changeset detectedLatest commit: 0a9f5bd The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Changeset File Check
|
Size Analysis ReportAffected Products
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code change is fine but I disagree a bit with the direction (but can certainly be convinced). We have our own internal data structures, and we shouldn't make an effort to make the "consumable" by our users. This makes our life more complicated and gives off the impression that we will support the output format going forward. It also convolutes the returned data, with very little additional benefit to the user.
Consider this: What does the user want when they call toJSON on a DocumentReference? They probably would prefer something like:
{
path: 'coll/doc',
firestore: {project: 'foo', database: '(default)'}
converter: null
}
over
{
path: 'coll/doc',
firestore: {project: 'foo', database: '(default)', asyncQueue: {...}, firestoreClient: {...} }
converter: null
}
The first one is more concise, provides the information the user likely needs, and is less likely to change over time.
In case you are wondering where my argument is going (and if it is going anywhere): I think we should just implement toJSON on FirebaseFirestore and call it a day. The toJSON should have enough information to help the developer distinguish it from other instances, that is - database ID and FirebaseApp (and maybe others, but select instance members).
And yes, this should also work for Firbease@exp.
FWIW, the change to FirebaseApp already follows this pattern.
Let me know if you want to talk about this more.
@schmidt-sebastian Thanks for the suggestion -- your proposal creates a much more simplified API surface and doesn't require us to maintain a bunch of toJSON() fields. I ended up only including the databaseId and settings, since the project/database fields are not available by default. I'm not sure if I should expand the individual fields or add a return type. WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add app
to Firebase.toJSON as well, but otherwise this LGTM. You might want to verify that you are on the right prettier before submitting (but you might be and the code style may be outdated)
JSON.stringify(documentSnapshot('foo/bar', { a: 1 }, true)); | ||
JSON.stringify(documentSnapshot('foo/bar', { a: 1 }, false)); | ||
JSON.stringify(documentSnapshot('foo/bar', null, true)); | ||
JSON.stringify(documentSnapshot('foo/bar', null, false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these four cases test different paths in a toJSON call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not anymore, removed.
@@ -134,6 +134,13 @@ export class FirebaseFirestore implements FirestoreService { | |||
return this._terminateTask; | |||
} | |||
|
|||
toJSON(): object { | |||
return { | |||
databaseId: this._databaseId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include the app as well? I think the developers might want to see the app name here, since it acts as the main identifier for all Firebase components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added app.name
to the JSON.
packages/app-types/index.d.ts
Outdated
@@ -112,6 +112,8 @@ export interface FirebaseNamespace { | |||
// Sets log handler for all Firebase components. | |||
onLog(logCallback: LogCallback, options?: LogOptions): void; | |||
|
|||
toJSON(): object; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also modify the Firestore and the Firebase types if you modify this (firestore-types/index.d.ts and firebase/index.d.ts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
71c14c3
to
7c31f91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Can you please add a test for toJSON()
in @firebase/app?
@Feiyang1 Done. Also added support for toJSON() in FirebaseAppLite. |
Co-authored-by: Feiyang <feiyangc@google.com>
Fixes #4258.
Adding
toJSON()
on the Firebase instance breaks the circular reference betweenfirebase.app.container
and the Firestore instance. Similarly, addingtoJSON()
to theAsyncQueue
andExponentialBackoff
allows them to be serialized despite the circular references.Some questions:
app-compat
?toJSON()
? I'm afraid of locking them in and causing a breaking change if we ever want to change them in the future.